Claude finished @fabrizzio-dotCMS's task in 3m 19s —— View job

PR Review

• Read changed files
• Analyze implementation correctness
• Check test coverage
• Post review feedback

Fix is sound for the two targeted hotspots. A few issues worth a second look:

1. isValidOneToOneRelationship() still uses getRelatedContent() — unfixed path

ESContentletAPIImpl.java:8809 (untouched):

List<Contentlet> relatedContents = getRelatedContent(
    contentsInRelationship.get(0), relationship, null,
    APILocator.getUserAPI().getSystemUser(), true, 1, 0, null);

This PR guards the main loop with if (ONE_TO_MANY) but doesn't touch isValidOneToOneRelationship, which still routes through RelationshipCache. For ONE_TO_ONE it's always at most 1 child, so the blast radius is limited — but if the environment also has ONE_TO_ONE relationships at scale, the cache-miss path is still live. Not a regression, but worth noting as a follow-up.

2. Existence check still hydrates a full Contentlet per row

dbRelatedContentByParent / dbRelatedContentByChild internally call APILocator.getContentletAPI().find(inode, ...) for every row returned (line 625 in RelationshipFactoryImpl.java). For the LIMIT 1 existence check, that's 2 round-trips (1 join query + 1 contentlet fetch) instead of the 20 000+ it was, which is a massive improvement — but a pure COUNT(*) or EXISTS query at the DB level would reduce it to 1 round-trip and avoid object allocation entirely. The current approach is acceptable for now, but it's an obvious next step if this path remains hot.

3. null orderBy passed without documentation

Both new calls pass null for orderBy:

dbRelatedContent(rel, contentlet, checkParent, false, null, 1, 0)

SQLUtil.sanitizeSortBy(null) returns null, UtilMethods.isSet(null) is false, so includeContentlet is false and the query falls back to ORDER BY t.tree_order, cvi.version_ts. This is safe, but it's a subtle dependency on internal fallback behavior. Passing "tree_order" explicitly (matching the existing convention in the codebase at lines 2376, 2471) would make the intent clear and future-proof against sanitizer changes.

4. Language semantics silently changed

Original getRelatedContent() routed through Elasticsearch and would have respected language context. The new dbRelatedContent() has no language filter in its SQL — it returns results across all languages. For the "does any related content exist?" check this is actually more correct (don't fail a required-relationship validation just because the current language differs). But the behavioral change is undocumented, and it's non-obvious that this is intentional rather than accidental.

5. Missing test: rejection path for required relationship with no DB content

The first test (validateRelationships_requiredField_existingDbRelation_doesNotThrow) validates that re-save succeeds when content exists in the DB. There's no test verifying the mirror case: a new contentlet (or one with no DB-side relationship) with a required field and an empty payload is still rejected. That's the path at lines 8645-8650. If someone accidentally inverted the isEmpty() check, the existing tests wouldn't catch it.

6. parent2 = next() in ONE_TO_MANY test is an unnamed stub, not a real saved entity

ESContentletAPIImplTest.java:5102:

final Contentlet parent2 = new ContentletDataGen(parentCT).setPolicy(IndexPolicy.FORCE).next();
// ...
contentletAPI.checkin(parent2, Map.of(relationship, list(child)), user, false);

parent2 has no identifier. The cardinality check at line 8709 evaluates !relatedContents.get(0).getIdentifier().equals(contentlet.getIdentifier()) where contentlet.getIdentifier() is null/empty. This comparison returns false (correctly triggering the violation), but for a different reason than intended — it's parent1Id.equals(null) rather than parent1Id.equals(parent2Id). The test would fail to detect a regression where the check uses inode or some other field instead of identifier. Persisting parent2 first would make this a genuine identity comparison.

Nothing in the core logic looks wrong. The two hotspots are correctly addressed, the cardinality guard for ONE_TO_MANY is sound, and catching only DotDataException (dropping DotSecurityException) is correct since dbRelatedContent doesn't declare it. The items above are improvements, not blockers — except #5 (missing rejection test) which is a real coverage gap.